Review of POS mempool (remove _connectFailingTxn) #1114

diamondhands0 · 2024-03-14T03:23:04Z

No description provided.

* PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment * Constants and network changes * Test MsgDeSoVerack encoding * Fix snapshot hack * Revert "Remove constants/network" This reverts commit b467ddbcd034c2e8d2728a7e77f4b714b686a760. * Fix compilation errors * Address review comments

* PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment * Another review round * gofmt * Comment change

* RemoteNode and RemoteNodeId * Add HandshakeController PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests Better connection testing framework Add validator integration test Fix validator-validator connection test; Add nonValidator-validator test * Review round * Add HandshakeController PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests Better connection testing framework Add validator integration test Fix validator-validator connection test; Add nonValidator-validator test * Final pass

* Add RemoteNodeIndexer * Add HandshakeController PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests Better connection testing framework Add validator integration test Fix validator-validator connection test; Add nonValidator-validator test Simplify indices Simplify remote node indexer; fix compilation Simplify RemoteNodeManager More RemoteNodeManager updates Nits

* Add HandshakeController PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests * Add HandshakeController PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests Better connection testing framework Add validator integration test Fix validator-validator connection test; Add nonValidator-validator test Simplify indices Simplify remote node indexer; fix compilation * Add HandshakeController PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests Better connection testing framework Add validator integration test Fix validator-validator connection test; Add nonValidator-validator test Simplify indices Simplify remote node indexer; fix compilation Simplify RemoteNodeManager * Merge HandshakeStage with RemoteNodeStatus; small HandshakeController nits * Nit * HandshakeController updates * Nits * Quick nit * Nits * Comment nit

PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment Integration testing updates PoS Block Producer: TxnConnectStatusByIndex (#672) * TransactionConnectStatus and ConnectFailingTransaction * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to 960001c. * Revert "Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions"" This reverts commit 10a147654c5147c28ec674d0650bb54c8d9cebce. * Revert "Merge branch 'p/bmf-status-connected' into p/failing-transactions" This reverts commit d3e543c4c3e6f03cc74087b05c268d4449ba1689, reversing changes made to a9f7827. * TransactionConnectStatus and ConnectFailingTransaction * Initial _connectFailingTransaction * ConnectFailingTransaction and GlobalParamsEntry updates * Fix merge conflicts * gofmt * Fix merge conflicts * Fix blockheight * Fix merge conflicts * gofmt * Revert connect failing transaction * Add TxnStatusConnectedIndex to block and header * Fix naming * Fix tests; remove asserts * Update comment RemoteNode and RemoteNodeId Initial remote node manager tests remote node tests Better connection testing framework Add validator integration test Fix validator-validator connection test; Add nonValidator-validator test Simplify indices Simplify remote node indexer; fix compilation Simplify RemoteNodeManager More RemoteNodeManager updates Nits

This reverts commit 831096ac1d3008233868ac8b8f0eca4cd2b9553e.

This reverts commit 0604b6d3fc155177a2bb295e6635ed21b20dd947.

…-syncing

* Revert "Code split" This reverts commit c0c32f3943ead0e06fdfb3343954a6b5273ea887. * Review * Sync trunk * Rename

* Revert "Another split" This reverts commit eaeec58. * Revert routine stops * gofmt * Add addrMgr to Server * Review

* Renames * nits * More renames * Review

* Some fixes * Fixes * Fix another integration test * Fix integration tests * Fix RegtestMiner

* noop * NetworkManager documentation * gofmt

* Fix Deadlock and Test AddIps * Glog fix

…-syncing

…utine (#1083)

diamondhands0 · 2024-03-14T03:30:28Z

lib/block_view.go

+				errors.Wrapf(err, "ConnectTransactions: Problem connecting txn %d on copy view", ii)
+		}
+
+		utxoOpsForTxn, totalInput, totalOutput, fee, err = bav.ConnectTransaction(


Connecting the txn twice is definitely going to slow us down. I almost wish there was a way to do this in such a way that we just update the pointer to point to the copiedView.

Also I'm not 100% sure we need this _connectTransactions primitive on the view vs doing this logic in the mempool one time but will keep reading.

diamondhands0 · 2024-03-14T03:30:55Z

lib/block_view.go

+		}
+		updateValues(utxoOpsForTxn, totalInput, totalOutput, fee, true)
+
+		if totalConnectedTxns == 0 {


I don't see how we would ever get past this line since totalConnectedTxns isn't incremented until AFTER this line. Could this be a bug?

diamondhands0 · 2024-03-14T03:31:13Z

lib/block_view.go

@@ -3952,104 +4035,6 @@ func (bav *UtxoView) ValidateTransactionNonce(txn *MsgDeSoTxn, blockHeight uint6
 	return nil
 }


Love this deletion.

diamondhands0 · 2024-03-14T03:33:59Z

lib/block_view.go

@@ -3920,6 +3920,89 @@ func (bav *UtxoView) _connectTransaction(
 	return utxoOpsForTxn, totalInput, totalOutput, fees, nil
 }

+func (bav *UtxoView) ConnectTransactions(


I wish there was a comment here explaining why we need this. You obviously added it for a reason and it would be super helpful to have that reason here so that when I review I can internalize what drove the addition quickly instead of having to figure it out. poolcoke has recently internalized this advice and started writing extremely long comments on everything. I love this and would like to see more of it from everyone. See here as an example all throughout this file:

core/lib/block_view_lockups.go

Line 114 in 3dc58f3

type LockedBalanceEntryKey struct {

diamondhands0 · 2024-03-14T03:34:48Z

lib/handshake_manager.go

@@ -1,168 +0,0 @@
-package lib


Love this deletion. I assume the logic is now embedded in one of the other parts of the code but will keep reading.

diamondhands0 · 2024-03-14T03:35:42Z

lib/network.go

}

All of these deletions are amazing. There was so much complexity driven by the changing block header that is now no longer needed. I really love it.

diamondhands0 · 2024-03-14T03:36:41Z

lib/network_manager.go

+		ValidatorIndex:                        collections.NewConcurrentMap[bls.SerializedPublicKey, *RemoteNode](),
+		NonValidatorOutboundIndex:             collections.NewConcurrentMap[RemoteNodeId, *RemoteNode](),
+		NonValidatorInboundIndex:              collections.NewConcurrentMap[RemoteNodeId, *RemoteNode](),
+		usedNonces:                            lru.NewCache(1000),


I assume this is to make it so that you can recognize when a validator is reusing a nonce? If yes I would bump it to a higher number like 100k since we could have a lot of validator connections coming in and a lot of nonces to track.

diamondhands0 · 2024-03-14T03:37:12Z

lib/network_manager.go

 func (nm *NetworkManager) _handleVersionMessage(origin *Peer, desoMsg DeSoMessage) {
-	nm.handshake.handleVersionMessage(origin, desoMsg)
+	if desoMsg.GetMsgType() != MsgTypeVersion {


Ok cool I see this is what you did with the handshake manager. I love this consolidation.

diamondhands0 · 2024-03-14T03:38:49Z

lib/peer.go

@@ -180,7 +180,7 @@ func (pp *Peer) HandleGetTransactionsMsg(getTxnMsg *MsgDeSoGetTransactions) {
 		// If the transaction isn't in the pool, just continue without adding
 		// it. It is generally OK to respond with only a subset of the transactions
 		// that were requested.
-		if mempoolTx == nil {
+		if mempoolTx == nil || !mempoolTx.IsValidated() {


Nice. I think this should prevent us from sending unvalidated txns on the first sync which is good.

diamondhands0 · 2024-03-14T03:40:24Z

lib/pos_block_producer.go

 		}
 		_, _, _, fees, err := blockUtxoViewCopy._connectTransaction(
 			txn.GetTxn(), txn.Hash(), uint32(newBlockHeight), newBlockTimestampNanoSecs,
 			true, false)

 		// Check if the transaction connected.
-		if err == nil {


Love this change. Just skipping txns that don't connect. Easy.

diamondhands0 · 2024-03-14T03:42:17Z

lib/pos_mempool.go

@@ -149,6 +154,11 @@ type PosMempool struct {
 	// based off the current state of the mempool and the most n recent blocks.
 	feeEstimator *PoSFeeEstimator

+	maxValidationViewConnects uint64


Here again I wish you had super detailed comments that explained what these control and all of the design decisions around them.

diamondhands0 · 2024-03-14T03:43:34Z

lib/pos_mempool.go

@@ -256,7 +269,6 @@ func (mp *PosMempool) Start() error {
 	// Create the transaction register, the ledger, and the nonce tracker,
 	mp.txnRegister = NewTransactionRegister()
 	mp.txnRegister.Init(mp.globalParams)
-	mp.ledger = NewBalanceLedger()
 	mp.nonceTracker = NewNonceTracker()


I was thinking maybe you don't need the nonce tracker anymore but I think you actually do need it to power replace-by-fee for txns that are beyond the 10k limit of our validation window. So good to keep it.

diamondhands0 · 2024-03-14T03:44:52Z

lib/pos_mempool.go

-
+// Any transaction that fails to add to the temp mempool will be removed from the main mempool.
+func (mp *PosMempool) validateTransactions() error {
+	mp.RLock()
 	if !mp.IsRunning() {


Normally when you have more than one line in a Lock/RLock we like to break it up into a function like:

func getThings() {
mp.RLock()
defer mp.RLock()

validationView := mp.readOnlyLatestBlockView mempoolTxns := mp.getTransactionsNoLock() return validationView, mempoolTxns

}

This prevents disasters as people modify the code in the future.

diamondhands0 · 2024-03-14T03:48:32Z

lib/pos_mempool.go

 	}
-	if err := tempPool.Start(); err != nil {
-		return errors.Wrapf(err, "PosMempool.refreshNoLock: Problem starting temp pool")
+	_, _, _, _, successFlags, err := copyValidationView.ConnectTransactionsWithLimit(txns, txHashes, uint32(mp.latestBlockHeight)+1, time.Now().UnixNano(),


I think I get why you made this function a member of the view but since we only really use it here, I would just embed the logic here and try to get it down to only one Connect call. I think it's slightly better from a readability standpoint, but from an efficiency standpoint the double Connect is definitely going to be a bottleneck so it fixes that.

To clarify: I'm suggesting you delete ConnectTransactionsWithLimit and embed the logic that it's running in here, but get rid of the second Connect.

diamondhands0 · 2024-03-14T03:50:14Z

lib/pos_mempool.go

-		if err == nil {
-			continue
+	for ii, successFlag := range successFlags {
+		if ii >= len(mempoolTxns) {


Interesting. How would ii ever be more than mempoolTxns if you're passing len(mempoolTxns) number of txns into the big COnnect above? A comment here would be good.

diamondhands0 · 2024-03-14T03:50:48Z

lib/pos_mempool.go

+		}
+		if successFlag {
+			mp.Lock()
+			mp.updateTransactionValidatedStatus(mempoolTxns[ii].Hash, true)


It looks like you have a double-locking bug here. You lock on the outside of updateTransactionValidationStatus and within the function as well.

diamondhands0 · 2024-03-14T03:53:07Z

lib/pos_mempool.go

 	}

-	return nil
+	txn.SetValidated(validated)


Since this is a write-lock, I'm worried this could cause a bottleneck. Just to max efficiency, I would try and do something more like this if you can:

// 1. With no lock, or just with RLock, get all the txns you want to mark as validated. txnsToMarkAsValidated = // whatever mp.Lock() // 2. Inside the lock blitz through these txns and call txn.SetValidated() on all of them in one go mp.Unlock()

It almost feels like you shouldn't even need to lock the whole mempool to update a single txn like this but if you're going to do it I'd just try to minimize the time spent holding the lock a little bit more, as I mentioned above.

diamondhands0 · 2024-03-14T03:54:18Z

lib/pos_mempool_ledger.go

@@ -1,93 +0,0 @@
-package lib


Great deletion.

diamondhands0 · 2024-03-14T03:54:51Z

lib/remote_node_indexer.go

@@ -1,46 +0,0 @@
-package lib


I could have sworn we deleted this in an earlier PR. Not sure why it's showing up here but hopefully I didn't mess anything up with my branch.

diamondhands0 · 2024-03-14T04:03:40Z

lib/server.go

@@ -1867,6 +1871,10 @@ func (srv *Server) _relayTransactions() {
 		// for which the minimum fee is below what the Peer will allow.
 		invMsg := &MsgDeSoInv{}
 		for _, newTxn := range txnList {
+			if !newTxn.IsValidated() {


I had to stare at this for a while and trace through things but I think it achieves everything we want:
0. We will not broadcast a txn to our peers until after it has passed validation, which is great.

We are waiting to bundle txns at regular intervals thanks to TransactionValidationRefreshIntervalMillis

If a user submits N txns to a single node in rapid succession, those should get bundled together IN ORDER thanks to the fact that the InvLis is an ordered list.

Do you see any reason why we wouldn't be achieving all of these properties with this approach? Also I would add all of this in a comment here to explain the full dynamics of what adding this line does.

diamondhands0 · 2024-03-14T04:11:32Z

lib/pos_mempool.go

 func (mp *PosMempool) startAugmentedViewRefreshRoutine() {
 	go func() {
+		mp.startGroup.Done()


The logic in this function seems to implement almost 1:1 what you're doing in ConnectTransactions[WithLimit]. Since you have to do some flavor of connect all these transactions to a view, I would consider a change as follows:
Instead of having ConnectTransactions ON the view, move it to pos_mempool.go over here with a signature something like the following:

func ConnectTransactionsToViewWithLimit(oldView *UtxoView, txnsToConnect []*MempoolTxn, numTxnsLimit int, ignoreFailing bool, etc...)

Then have this function simply return a new view that has connected everything to a copy of oldView. And you can use that function here and in the place where you're currently using COnnectTransactions. The upshot of this approach is you avoid adding code to the view and, critically, you only do ONE connect per txn because you don't need to update the internal state of a view.

diamondhands0 · 2024-03-14T04:16:40Z

lib/pos_mempool.go

+			mp.updateTransactionValidatedStatus(mempoolTxns[ii].Hash, true)
+			mp.Unlock()
+		} else {
+			txnsToRemove = append(txnsToRemove, mempoolTxns[ii])
 		}


Below this line you call removeTransactionNoLock(). Is that OK to do here given that you're not holding any kind of lock in the outer function? It feels like maybe you need to hold a mp.Lock() while you do it. Or call the RemoveTransaction function or something.

tholonious and others added 30 commits December 15, 2023 13:52

Add BLS Keystore

fefb100

Add validator deduplication test (#942)

21ba1dd

Revert "Code split" (#943)

59af74d

This reverts commit 831096ac1d3008233868ac8b8f0eca4cd2b9553e.

Revert "Code split" (#944)

3901564

This reverts commit 0604b6d3fc155177a2bb295e6635ed21b20dd947.

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

4a53182

…-syncing

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

c057262

…-syncing

Nits

3543f2c

PoS NetworkManager RemoteNode Cleanup (#945)

927df23

* Revert "Code split" This reverts commit c0c32f3943ead0e06fdfb3343954a6b5273ea887. * Review * Sync trunk * Rename

PoS NetworkManager Address (#957)

f60ec17

* Revert "Another split" This reverts commit eaeec58. * Revert routine stops * gofmt * Add addrMgr to Server * Review

PoS NetworkManager Rename and Nits (#959)

baa91ae

* Renames * nits * More renames * Review

PoS NetworkManager Fix Integration Tests (#960)

c7317f5

* Some fixes * Fixes * Fix another integration test * Fix integration tests * Fix RegtestMiner

Fix fmt (#973)

edaf174

PoS Networking and Syncing Documentation (#974)

76bf439

* noop * NetworkManager documentation * gofmt

Rewording (#981)

a6b26b7

Rename (#986)

20f2eb4

PoS NetworkManager Fix Deadlock and Test AddIps (#996)

5ffc022

* Fix Deadlock and Test AddIps * Glog fix

PoS NetworkManager Comment Nit (#997)

1b0bc01

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

b4454e0

…-syncing

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

c9571c8

…-syncing

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

f4059c6

…-syncing

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

362d634

…-syncing

Add ProtocolVersion To Regtest Params (#1035)

aa0fb85

Merge branch 'feature/proof-of-stake' into feature/pos-networking-and…

2a1e9c7

…-syncing

Fix Constructor Name for NetworkManager

f0c7b5d

lazynina and others added 6 commits February 28, 2024 18:21

Add support for seed hex in block producer seed (#1072)

1f644e8

Make GetCommittedTip public (#1073)

0c40ac8

Assorted small enhancements for node restart w/ pos (#1079)

9445f77

Fix txindex dependent transaction within uncommitted blocks (#1082)

e1312f9

Add flag to control number of mempool txns to sync in state syncer ro…

ea2db8f

…utine (#1083)

squash mempool commits

fa5220d

diamondhands0 requested a review from a team as a code owner March 14, 2024 03:23

diamondhands0 commented Mar 14, 2024

View reviewed changes

lib/pos_mempool_ledger.go

@@ -1,93 +0,0 @@

package lib

Copy link

Member Author

diamondhands0 Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great deletion.

diamondhands0 commented Mar 14, 2024

View reviewed changes

Base automatically changed from feature/pos-networking-and-syncing to feature/proof-of-stake March 20, 2024 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review of POS mempool (remove _connectFailingTxn) #1114

Review of POS mempool (remove _connectFailingTxn) #1114

diamondhands0 commented Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

diamondhands0 Mar 14, 2024

		@@ -3952,104 +4035,6 @@ func (bav UtxoView) ValidateTransactionNonce(txn MsgDeSoTxn, blockHeight uint6
		return nil
		}

Review of POS mempool (remove _connectFailingTxn) #1114

Are you sure you want to change the base?

Review of POS mempool (remove _connectFailingTxn) #1114

Conversation

diamondhands0 commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment